Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix use RequestStack instead of Request #118

Closed
wants to merge 4 commits into from

Conversation

upskaling
Copy link
Contributor

I tried to fix issues #65

Let me know if I can improve Pull requests

@matthijsch
Copy link
Contributor

Thanks! Can you fix the unit test?

@upskaling
Copy link
Contributor Author

@matthijsch If there is anything else you want me to correct, don't hesitate.

{
$this->request = $request;
$this->request = $request->getCurrentRequest();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't call getCurrentRequest() in the constructor, but in the method in which it is used. The reason is, that the constructor is called during service generation (when it is injected into another service) but the actual usage may be later, so the "current request" may be another one at that point.

https://symfony.com/doc/current/service_container/request.html

@upskaling
Copy link
Contributor Author

Thanks for taking the time to look at my code
I've made the changes, please let me know if there's anything else.

@chamaloriz
Copy link

@jdreesen @matthijsch @upskaling this is awesome ! Looking forward to it being merged !

@matthijsch matthijsch closed this May 8, 2024
@matthijsch matthijsch reopened this May 8, 2024
@matthijsch matthijsch closed this May 8, 2024
@matthijsch matthijsch reopened this Jun 19, 2024
@matthijsch matthijsch closed this Jun 19, 2024
jaapromijn pushed a commit to jaapromijn/cookie-consent-bundle that referenced this pull request Jun 20, 2024
jaapromijn pushed a commit to jaapromijn/cookie-consent-bundle that referenced this pull request Jun 20, 2024
jaapromijn pushed a commit to jaapromijn/cookie-consent-bundle that referenced this pull request Jun 20, 2024
@matthijsch
Copy link
Contributor

Sadly we seem to be experiencing some issues with Github and PR's were automatically closed and can't be reopened. Thank you for your work, I've created a new PR #125 and also replaced RequestStack for the logger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants